remove need for nil checks around statsd metrics #932
Merged
+25
−25
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmhodges
force-pushed
the
statsd-null-object
branch
from
7659ff5 to
25b0a27
Compare
jmhodges
changed the title
jmhodges
force-pushed
the
statsd-null-object
branch
from
25b0a27 to
a88ed97
Compare
jmhodges
requested review from
oskirby,
eviljeff and
hneiva
and removed request for
a team
bhearsum
approved these changes
Jul 31, 2024
We'd hopefully find out because our alerts would fire with a "not getting data" alarm. Def the next step is to make sure we have alerts in place using this data! |
willdurand
approved these changes
Jul 31, 2024
Currently, we have to check if `autographer.stats` is nil anywhere we want to emit a statsd metric. This is error-prone and verbose. Work for AUT-150 (and more) would be easier if we do this here. This patch makes the `autographer.stats` field always non-nil. By default, `autographer`s created with `newAutographer` will have a `statsd.NoOpClient` object for its `stats` and if the autograph config has a statsd server configured, it will be replaced with a real statsd client when `autographer.addStats` is called. That addStats call will now always occur in autograph's main function. This moves the conditional for checking if the statsd server is configured into autographer.addStats. However, it would probably be better in the future for us to have this and the similar `autograph.add*` methods moved into the `newAutographer` contructor. This, though, at least makes the autographer type a lil safer on construction. This means that we use `statds.ClientInterface` instead of `*statsd.Client` everywhere to fit both client types. Along the way, we also have to fix up the main_test.go that was reproducing that conditional from the main inside it. We also fix an signer/xpi test that was depending on a statsd server running but not actually needing it to finish. This patch doesn't remove all of the nil checks. A follow up will be made to do so. Updates AUT-159 Updates AUT-150
jmhodges
force-pushed
the
statsd-null-object
branch
from
934e070 to
4260381
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we have to check if
autographer.statsis nil anywhere wewant to emit a statsd metric. This is error-prone and verbose. Work for
AUT-150 (and more) would be easier if we do this here.
This patch makes the
autographer.statsfield always non-nil. Bydefault,
autographers created withnewAutographerwill have astatsd.NoOpClientobject for itsstatsand if the autograph confighas a statsd server configured, it will be replaced with a real statsd
client when
autographer.addStatsis called. That addStats call willnow always occur in autograph's main function.
This moves the conditional for checking if the statsd server is
configured into autographer.addStats. However, it would probably be
better in the future for us to have this and the similar
autograph.add*methods moved into thenewAutographercontructor.This, though, at least makes the autographer type a lil safer on
construction.
This means that we use
statds.ClientInterfaceinstead of*statsd.Clienteverywhere to fit both client types.Along the way, we also have to fix up the main_test.go that was
reproducing that conditional from the main inside it. We also fix an
signer/xpi test that was depending on a statsd server running but not
actually needing it to finish.
This patch doesn't remove all of the nil checks. A follow up will be
made to do so.
Updates AUT-159
Updates AUT-150